-
Notifications
You must be signed in to change notification settings - Fork 1.5k
YDB database support #1215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
YDB database support #1215
Conversation
YDB Support
YDB Reimplement Lock Mechanics
|
@dhui Can you see this PR? We are waiting YDB support in golang-migrate very much |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for YDB (Yandex Database) to golang-migrate, implementing a complete database driver with migration capabilities, locking mechanisms, and comprehensive test coverage.
Key Changes:
- New YDB database driver with full migration lifecycle support (Open, Close, Lock, Unlock, Run, SetVersion, Version, Drop)
- Extensive test suite including integration tests with Docker containers
- Complete documentation with connection string format and configuration options
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
internal/cli/build_ydb.go |
Build tag configuration to conditionally include YDB driver in CLI builds |
go.mod / go.sum |
Added YDB SDK dependencies (v3.100.0) and related transitive dependencies |
database/ydb/ydb.go |
Main driver implementation with connection handling, migrations, versioning, and locking |
database/ydb/ydb_test.go |
Comprehensive test suite with Docker-based integration tests for multiple YDB versions |
database/ydb/examples/migrations/*.sql |
Example migrations demonstrating YDB-specific features (tables, columns, indexes, topics) |
database/ydb/README.md |
Detailed documentation covering connection strings, authentication methods, TLS options, and troubleshooting |
README.md |
Added YDB to the list of supported database drivers |
Makefile |
Added ydb to the DATABASE variable for build/test targets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| If golang-migrate fails to acquire the lock and no migrations are currently running. | ||
| This may indicate that one of the migrations did not complete successfully. |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sentence is incomplete. Consider revising to: "If golang-migrate fails to acquire the lock when no migrations are currently running, this may indicate that one of the migrations did not complete successfully." This makes it a complete, grammatically correct sentence.
| If golang-migrate fails to acquire the lock and no migrations are currently running. | |
| This may indicate that one of the migrations did not complete successfully. | |
| If golang-migrate fails to acquire the lock when no migrations are currently running, this may indicate that one of the migrations did not complete successfully. |
| if err := tx.Commit(); err != nil { | ||
| return &database.Error{OrigErr: err, Err: "transaction commit failed"} | ||
| } | ||
| return err |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function should return nil on success, not err (which is nil at this point but makes the intent unclear). Change to return nil for consistency and clarity.
| return err | |
| return nil |
| var ( | ||
| ErrNilConfig = fmt.Errorf("no config") | ||
| ErrNoDatabaseName = fmt.Errorf("no database name") | ||
| ErrUnsupportedTLSVersion = fmt.Errorf("unsupported tls version: use 1.0, 1.1, 1,2 or 1.3") |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in error message: "1,2" should be "1.2" to be consistent with the other version numbers in the list.
| ErrUnsupportedTLSVersion = fmt.Errorf("unsupported tls version: use 1.0, 1.1, 1,2 or 1.3") | |
| ErrUnsupportedTLSVersion = fmt.Errorf("unsupported tls version: use 1.0, 1.1, 1.2 or 1.3") |
| case err != nil: | ||
| return 0, false, &database.Error{OrigErr: err, Query: []byte(getVersionQuery)} | ||
| default: | ||
| return int(v), dirty, err |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function returns err in the default case, but at this point err is always nil (since we didn't hit the error cases above). This should be return int(v), dirty, nil for clarity and correctness.
| return int(v), dirty, err | |
| return int(v), dirty, nil |
| getLockQuery := fmt.Sprintf("SELECT * FROM %s WHERE lock_id = '%s'", y.config.LockTable, aid) | ||
| rows, err := tx.Query(getLockQuery, aid) |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL query at line 315 already has aid interpolated (WHERE lock_id = '%s'), but line 316 attempts to pass aid as an additional parameter to tx.Query. This will cause an error or unexpected behavior. Either:
- Remove the
aidparameter from thetx.Querycall (line 316), or - Use parameterized queries with placeholders instead of string interpolation (recommended for security).
The same issue exists at line 332 where the query has aid interpolated but the parameters should be passed separately.
| config.LockTable = defaultLockTable | ||
| } | ||
|
|
||
| conn, err := instance.Conn(context.TODO()) |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses both context.TODO() (lines 76, 126, 225, 261, 349) and context.Background() (lines 210, 300, 369, 399) inconsistently. For consistency with other database drivers in this codebase (e.g., Postgres uses context.Background() throughout), consider using context.Background() consistently across all operations.
| conn, err := instance.Conn(context.TODO()) | |
| conn, err := instance.Conn(context.Background()) |
| deleteVersionQuery := fmt.Sprintf(` | ||
| DELETE FROM %s | ||
| `, y.config.MigrationsTable) | ||
|
|
||
| insertVersionQuery := fmt.Sprintf(` | ||
| INSERT INTO %s (version, dirty, created) VALUES (%d, %t, CurrentUtcTimestamp()) | ||
| `, y.config.MigrationsTable, version, dirty) | ||
|
|
||
| tx, err := y.conn.BeginTx(context.TODO(), &sql.TxOptions{}) | ||
| if err != nil { | ||
| return &database.Error{OrigErr: err, Err: "transaction start failed"} | ||
| } | ||
|
|
||
| if _, err := tx.Exec(deleteVersionQuery); err != nil { | ||
| if errRollback := tx.Rollback(); errRollback != nil { | ||
| err = multierror.Append(err, errRollback) | ||
| } | ||
| return &database.Error{OrigErr: err, Query: []byte(deleteVersionQuery)} | ||
| } | ||
|
|
||
| // Also re-write the schema version for nil dirty versions to prevent | ||
| // empty schema version for failed down migration on the first migration | ||
| // See: https://github.com/golang-migrate/migrate/issues/330 | ||
| if version >= 0 || (version == database.NilVersion && dirty) { | ||
| if _, err := tx.Exec(insertVersionQuery, version, dirty); err != nil { |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two issues with this implementation:
- The SQL query uses string interpolation for values (
%d,%t) at line 222, which is a security concern and should use parameterized queries instead. - Line 241 attempts to pass
versionanddirtyas parameters totx.Exec, but the query already has these values interpolated, causing a mismatch.
The correct approach is to build the query without interpolating the values (use placeholders like ? or positional parameters if YDB supports them), then pass version and dirty as parameters to the Exec call. This is also the pattern used in other database drivers (e.g., Postgres uses $1, $2).
Added YDB support to golang-migrate
This PR implements support for YDB in golang-migrate. The main changes include:
How to Test: